-
Notifications
You must be signed in to change notification settings - Fork 152
Feature/bulk sending #44
base: master
Are you sure you want to change the base?
Conversation
* Adds a handler | ||
* | ||
* @param $osType | ||
* @param $service | ||
*/ | ||
public function addHandler($osType, $service) | ||
public function addHandler(string $osType, OS\OSNotificationServiceInterface $service) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't typehint for string
queue() queues messages without calling sendMessages(), so you can then call sendMessages manually. Refactored send to call queue() then sendMessages(). Tidied code a little to conform to PSR-2
I think this is a quite important feature actually. How can we accelerate the work on this? |
@rjmunro are you able to look at this again? |
can't this just be done by combining this library with a delayed job kinda library like BCCResqueBundle? |
@abbood The push message providers offer nice bulk sending capabilities, but we are not using them. Adding a queue library would not help with this. |
Does this bundle work with multiple messages? I'm viewing the source code but really, I can't understand! |
|
||
$handler = $this->handlers[$message->getTargetOS()]; | ||
|
||
if (method_exists($handler, "queue")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be nice to create a BulkNotificationInterface
instead of doing this ?
@rjmunro Whats the status of this? |
ping ? |
@rjmunro How is this issue? tks. |
I'm not able to do any more work on this. If someone wants to take it over, please do. |
Begin work on bulk sending of push messages. Not ready for merging yet, just posting for comment purposes.
Depends on: #36 being merged first.